-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: add process tree data source #3488
Conversation
ce696ac
to
620da57
Compare
620da57
to
f4b8f28
Compare
types/datasource/proctree.go
Outdated
ForkTime time.Time | ||
ExitTime time.Time | ||
Name string | ||
ProcessHash uint32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this hash from the "thread group leader" ? Maybe we should use the same names both sides (process tree and here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this connects to my previous comment.
There should be a clear distinction between processes and threads.
I think we have too much of a mish-mash between kernel terms and user terms.
This should be the process hash, because the user is interested in the process information. Not necessarily the thread group leader information (the TaskInfo
of the group leader is irrelevant for most use cases of the process tree).
** I took a closer look on the current design, and I see that some crucial process information is accessible only through the TaskInfo
struct of the process. We need to discuss the design here to think what is the best API here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, just need to decide on the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in the end that having more user-friendly names here is more appropriate.
Tell me what you think.
Hey Alon, I think we're close. Overall I like the datasource, the way you're accessing the process tree, etc. I have some doubts on why you're not distinguishing between threads and processes in the signature (you can answer in those comments). I'll get back to this tomorrow as soon as you answer/re-push things. Please check the tests results as well. Cheers! |
Answering #3488 (comment) |
Has errors with the e2e tests when trying to find the thread hash in the tree. |
74bc00f
to
f55a435
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
needs types PR to be merged first
f55a435
to
4e65751
Compare
#3509 merged, feel free to rebase. |
5358546
to
49d8fd7
Compare
@yanivagman @rafaeldtinoco needs someone to restart the tests |
49d8fd7
to
8b6b540
Compare
My local e2e test is not working. EDIT:
|
88ad33d
to
f63ea9b
Compare
Like we've just discussed, the only way to get rid of this would be to get "both" sources of events enabled (with #3498). Then the proctree entry would always exist since the fork/exec/exit events would come in the same pipeline (at least for this test that doesn't have a filled pipeline). I'm pretty sure in regular situations the signal events will happen first, but, still ... its an event oriented architecture, can't guarantee a transaction with unordered events without holding the pipeline. |
I decided to make this PR only on the data source, and putting the e2e tests on another PR. |
f63ea9b
to
ec01ae8
Compare
ec01ae8
to
87e4fda
Compare
Expose the process tree to the signatures using a datasource. This way a signatures can use the process tree to have state, instead of using multiple events and saving the state internally.
87e4fda
to
aa2f014
Compare
Rebased at #3522 (solving conflicts mostly) |
1. Explain what the PR does
ce696ac chore(proctree): add process tree e2e tests
7098db0 feat(proctree): create process tree data source